flags/abilityflagset.go (1)

21-37: ⚠️ Potential issue | 🟡 Minor

Suppress FlagSet's default error output.

With flag.ContinueOnError, FlagSet.Parse() writes errors to the FlagSet's output (defaulting to os.Stderr) before returning the error. Since callers in ability.go, pokemon.go, and tcg.go format and return "error parsing flags: %v" through the normal output path, users see competing error messages on stderr: one from the FlagSet framework and one from the caller's formatting.

Call SetOutput(io.Discard) on each FlagSet after creation to suppress the default error printing and ensure errors flow through a single, controlled path:

Sketch

 func SetupAbilityFlagSet() *AbilityFlags {
   af := &AbilityFlags{}
   af.FlagSet = flag.NewFlagSet("abilityFlags", flag.ContinueOnError)
+  af.FlagSet.SetOutput(io.Discard)

Apply the same fix to SetupPokemonFlagSet() and SetupTcgFlagSet().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flags/abilityflagset.go` around lines 21 - 37, The FlagSet created in
SetupAbilityFlagSet (af.FlagSet) is using flag.ContinueOnError which still
prints errors to the FlagSet's output (default os.Stderr); call
af.FlagSet.SetOutput(io.Discard) right after creating the FlagSet to suppress
that automatic printing so Parse() errors are only handled by the callers. Apply
the same change to the other factories SetupPokemonFlagSet and SetupTcgFlagSet
to ensure all FlagSet instances stop writing directly to stderr.

cmd/pokemon/pokemon_test.go (1)

95-112: ⚠️ Potential issue | 🟡 Minor

Assert the expected error outcome.

The new invalid-flag case sets expectedError: true, but the test discards PokemonCommand’s error, so it only verifies output text.

🧪 Proposed fix

-			output, _ := PokemonCommand()
+			output, err := PokemonCommand()
 			cleanOutput := styling.StripANSI(output)
 
+			if tt.expectedError {
+				assert.Error(t, err, "Expected an error")
+			} else {
+				assert.NoError(t, err, "Expected no error")
+			}
 			assert.Equal(t, tt.expectedOutput, cleanOutput, "Output should match expected")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/pokemon/pokemon_test.go` around lines 95 - 112, The test currently
ignores the error returned by PokemonCommand(), so add error handling: capture
output, err := PokemonCommand() inside the t.Run closure and assert that err is
non-nil when tt.expectedError is true (use assert.Error) and nil when false (use
assert.NoError), then continue to compare the cleaned output to
tt.expectedOutput; update the test case loop that calls PokemonCommand() and the
assertions accordingly to reference the existing tt.expectedError and
PokemonCommand symbols.

cli_test.go (1)

141-147: ⚠️ Potential issue | 🟠 Major

Set os.Args for command-handler cases.

The new "Missing Pokémon name" case reaches PokemonCommand, which reads global os.Args; this loop currently only passes tt.args to runCLI. Mirror TestRunCLI so the test validates the intended CLI argv instead of the Go test process args.

🧪 Proposed fix

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			originalArgs := os.Args
+			os.Args = append([]string{"poke-cli"}, tt.args...)
+			defer func() { os.Args = originalArgs }()
+
 			exitCode := runCLI(tt.args)
 			if exitCode != tt.expected {
 				t.Errorf("expected %d, got %d for args %v", tt.expected, exitCode, tt.args)
 			}
 		})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli_test.go` around lines 141 - 147, The test loop fails to set the process
argv used by PokemonCommand because runCLI(tt.args) only passes args to the
function but PokemonCommand reads global os.Args; update the test to set os.Args
= append([]string{os.Args[0]}, tt.args...) (or mirror the approach used in
TestRunCLI) before calling runCLI(tt.args), and restore the original os.Args
after the subtest to avoid leaking state; ensure this change targets the test
cases including "Missing Pokémon name" so PokemonCommand sees the intended argv.

cmd/utils/validateargs.go (1)

78-88: ⚠️ Potential issue | 🟠 Major

Guard empty trailing arguments before indexing arg[0].

poke-cli pokemon pikachu "" can panic here because arg[0] is evaluated for an empty string. Treat empty args like invalid/empty flags before checking the first byte.

🐛 Proposed fix

 	if len(args) > 3 {
 		for _, arg := range args[3:] {
 			// Check for an empty flag after Pokémon's name
-			if arg == "-" || arg == "--" {
+			if arg == "" || arg == "-" || arg == "--" {
 				return fmt.Errorf("%s", FormatError(fmt.Sprintf("Empty flag '%s'.\nPlease specify valid flag(s).", arg)))
 			}
 
 			// Check if the argument after Pokémon's name is an attempted flag
-			if arg[0] != '-' {
+			if !strings.HasPrefix(arg, "-") {
 				return fmt.Errorf("%s", FormatError(fmt.Sprintf("Invalid argument '%s'.\nOnly flags are allowed after declaring a Pokémon's name", arg)))
 			}
 		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/utils/validateargs.go` around lines 78 - 88, The loop over args[3:]
indexes arg[0] without guarding empty strings, which can panic; update the check
in the loop inside validateargs.go to first treat empty string args (len(arg) ==
0 or arg == "") as an empty/invalid flag and return the same formatted error as
for "-" or "--", then proceed to the existing checks (keep the "-"/"--" check
and only access arg[0] after confirming arg is non-empty). Refer to the loop
that iterates over args[3:], the arg variable, and the
FormatError(fmt.Sprintf(...)) error returns to apply this ordering change.

flags/pokemonflagset.go (1)

62-97: ⚠️ Potential issue | 🟡 Minor

Suppress flagset's direct output to keep parse diagnostics inside the command output path.

With flag.ContinueOnError, the custom Usage function in pokemonflagset.go (line 84-97) still prints directly via fmt.Println, bypassing the command's error buffer in PokemonCommand. When Parse encounters an invalid flag, it invokes Usage before returning the error, so parse errors emit unformatted output to stdout outside the centralized output path.

Disable the flagset's own output before parsing:

Fix: Suppress flagset output in the parse path

 	pf := flags.SetupPokemonFlagSet()
+	pf.FlagSet.SetOutput(io.Discard)
+	pf.FlagSet.Usage = func() {}
 
 	args := os.Args

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flags/pokemonflagset.go` around lines 62 - 97, The custom Usage currently
prints directly with fmt.Println, bypassing the command's error buffer; change
the Usage implementation to write to the flagset's output (use
fmt.Fprintln(pf.FlagSet.Output(), helpMessage) instead of fmt.Println) and, in
the parse path where the FlagSet is parsed (e.g., where PokemonCommand calls
pf.FlagSet.Parse or similar), call pf.FlagSet.SetOutput(io.Discard) before
parsing so the flag package won't emit unformatted output on parse errors; this
keeps output suppression centralized while still allowing Usage to be captured
if you set a non-discard output later.

cmd/berry/berry_test.go (2)

84-116: ⚠️ Potential issue | 🟡 Minor

Actually send the table-driven key messages.

This test defines escape/ctrl+c cases but calls m.Update(nil) for every case, so it does not verify the migrated tea.KeyPressMsg quit handling.

🧪 Proposed fix

 	tests := []struct {
-		name        string
-		keyMsg      string
-		shouldQuit  bool
-		expectError bool
+		name       string
+		msg        tea.KeyPressMsg
+		shouldQuit bool
 	}{
 		{
 			name:       "escape key",
-			keyMsg:     "esc",
+			msg:        tea.KeyPressMsg{Code: tea.KeyEscape},
 			shouldQuit: true,
 		},
 		{
 			name:       "ctrl+c key",
-			keyMsg:     "ctrl+c",
+			msg:        tea.KeyPressMsg{Code: 'c', Mod: tea.ModCtrl},
 			shouldQuit: true,
 		},
 		{
 			name:       "other key",
-			keyMsg:     "j",
+			msg:        tea.KeyPressMsg{Code: 'j'},
 			shouldQuit: false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			updated, _ := m.Update(nil)
+			updated, cmd := m.Update(tt.msg)
+			got := updated.(model)
 
-			if tt.shouldQuit {
-				if updated == nil {
-					t.Errorf("Update() returned nil model")
-				}
+			if got.quitting != tt.shouldQuit {
+				t.Errorf("Update() quitting = %v, want %v", got.quitting, tt.shouldQuit)
+			}
+			if tt.shouldQuit && cmd == nil {
+				t.Error("Update() should return a quit command")
 			}
 		})
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/berry/berry_test.go` around lines 84 - 116, The table-driven test
currently calls m.Update(nil) and never sends the key events to verify quit
behavior; change the subtest to construct and pass the appropriate
tea.KeyMsg/tea.KeyPressMsg (based on tt.keyMsg values "esc", "ctrl+c", "j") into
m.Update so Update receives the key input to process; locate the loop and
replace the nil argument in the call to m.Update(nil) with a mapped message
(e.g. map "esc" -> tea.KeyMsg for Esc, "ctrl+c" -> Ctrl+C key message, others ->
rune/keypress) and assert the returned model/quit behavior as before.

---

249-257: ⚠️ Potential issue | 🟡 Minor

Avoid asserting the stale cached selection after immediate quit.

Update returns on ctrl+c before syncing selectedOption, so this test will see "" even though the table’s selected row is still Aguav.

🧪 Proposed fix

 	final := testModel.FinalModel(t).(model)
 
 	// First row should be selected by default
-	if final.selectedOption != "Aguav" {
-		t.Errorf("Expected initial selectedOption to be 'Aguav', got %q", final.selectedOption)
+	row := final.table.SelectedRow()
+	if len(row) == 0 || row[0] != "Aguav" {
+		t.Errorf("Expected initial selected row to be 'Aguav', got %v", row)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/berry/berry_test.go` around lines 249 - 257, The test is asserting
final.selectedOption right after sending ctrl+c, but Update returns before
selectedOption is synced; instead, read the table's selection directly from the
final model (e.g., use final.table.Cursor() or the table's
SelectedRow()/SelectedIndex() accessor on the model returned by
testModel.FinalModel(t)) or remove the stale selectedOption assertion — update
the assertion to use the table's selection API (referencing testModel.Send,
testModel.WaitFinished, testModel.FinalModel(t), and the model.table accessor)
so the test inspects the actual table cursor rather than the
potentially-unsynced selectedOption field.

cmd/berry/berry.go (1)

3-15: ⚠️ Potential issue | 🟠 Major

Return the query error instead of terminating the process.

tableGeneration is called from BerryCommand, which expects to append and return errors. log.Fatalf exits immediately, so callers/tests cannot handle database failures.

🐛 Proposed fix

 import (
 	"flag"
 	"fmt"
-	"log"
 	"os"
 	"strings"
 
@@
 	if err != nil {
-		log.Fatalf("Failed to get berry names: %v", err)
+		return fmt.Errorf("failed to get berry names: %w", err)
 	}

Also applies to: 125-127

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/berry/berry.go` around lines 3 - 15, tableGeneration currently calls
log.Fatalf which exits the process; change it to return an error instead and
propagate it so callers (like BerryCommand) can append/return errors. Update
tableGeneration's signature to return (table.Table, error) or error (as
appropriate), replace log.Fatalf and any other direct process exits (including
the occurrences around the 125-127 area) with fmt.Errorf(...) returns that
include contextual info, and update BerryCommand to check the returned error
from tableGeneration and append/return it rather than letting the program exit.
Ensure all call sites are updated to handle the new error return.

cmd/ability/ability_test.go (1)

55-72: ⚠️ Potential issue | 🟡 Minor

Assert the returned error for the new invalid-flag case.

wantError: true is set, but Line 69 discards AbilityCommand()’s error, so this test can pass even if invalid flags stop returning errors.

Proposed fix

-			output, _ := AbilityCommand()
+			output, err := AbilityCommand()
 			cleanOutput := styling.StripANSI(output)
 
+			if tt.wantError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
 			assert.Equal(t, tt.expectedOutput, cleanOutput, "Output should match expected")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/ability/ability_test.go` around lines 55 - 72, The test sets wantError:
true but discards the error from AbilityCommand(); update the test loop to
capture the returned error (e.g., output, err := AbilityCommand()) and assert it
against tt.wantError (use assert.Error when tt.wantError is true and
assert.NoError when false, or assert.Equal on (err != nil) to tt.wantError).
Modify the block that currently calls AbilityCommand() and assigns to output
only so it checks tt.wantError using the err value while still stripping and
comparing output when appropriate; reference AbilityCommand, tt.wantError, and
the test loop/t.Run to locate the change.

cmd/types/types_test.go (1)

150-153: ⚠️ Potential issue | 🟡 Minor

This subtest performs no assertion.

m.selectedOption = "Fire" is set but m.View() is never called and no assertion is made, so this subtest always passes without actually verifying behavior. Either remove it or complete the assertion.

Proposed fix

 	t.Run("View should return empty string when selectedOption is set", func(t *testing.T) {
 		m := createTestModel()
 		m.selectedOption = "Fire"
+		view := m.View()
+		assert.Equal(t, "", view.Content, "View should return empty string when selectedOption is set")
 	})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/types/types_test.go` around lines 150 - 153, The subtest creates a model
with createTestModel() and sets m.selectedOption = "Fire" but never calls
m.View() or asserts the result, so it should be completed: call m.View() on the
model (m.View()) and add an assertion (e.g., require.Equal / t.Fatalf /
t.Errorf) that the returned string is the expected empty string; alternatively
remove the entire subtest if it's redundant. Update the subtest that currently
references createTestModel, m.selectedOption and m.View to include the actual
assertion or delete it.

cmd/tcg/dashboard.go (1)

31-54: ⚠️ Potential issue | 🟠 Major

Use Bubble Tea v2's BackgroundColorMsg instead of synchronous lipgloss.HasDarkBackground().

lipgloss.HasDarkBackground(os.Stdin, os.Stdout) at line 34 performs a blocking terminal query (OSC 11) and reads the response from stdin. In a Bubble Tea v2 program, this can race with Bubble Tea's input loop, leak escape sequences into the rendered UI, or block indefinitely on terminals that don't respond. Bubble Tea v2 automatically publishes the detected background as a tea.BackgroundColorMsg event—the recommended approach for theme detection.

Defer style initialization to the Update method by handling tea.BackgroundColorMsg. In Init(), return tea.RequestBackgroundColor to trigger the background detection, then create styles with the resolved background color in Update when you receive the message. Store both light and dark color variants in the model if needed, or create a function like newStyles(bgIsDark bool) that accepts the background state.

Similar issues exist in styling/styling.go and styling/list.go at package init time.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/tcg/dashboard.go` around lines 31 - 54, The current newStyles() uses
lipgloss.HasDarkBackground(os.Stdin, os.Stdout) which blocks; change
initialization to use Bubble Tea v2's async background detection by returning
tea.RequestBackgroundColor from Init(), handle tea.BackgroundColorMsg in
Update(), and call a refactored constructor newStyles(bgIsDark bool) (or pass
the boolean into styles creation) when the BackgroundColorMsg arrives; update
the model to hold light/dark variants or constructed styles and remove any
synchronous lipgloss.HasDarkBackground calls (also fix similar package-init
usage in styling/styling.go and styling/list.go).